bpo-38724: Implement subprocess.Popen.__repr__ by dorosch · Pull Request #17151 · python/cpython

dorosch · 2019-11-14T10:30:37Z

https://bugs.python.org/issue38724

the-knights-who-say-ni · 2019-11-14T10:30:41Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@dorosch

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

cool-RR

Thanks for working on this.

I think that the PID by itself is not useful enough for identifying what's going on. I'd suggest including the args (truncated if they're beyond a certain length) and the return code if the process is finished.

Also, tests.

dorosch · 2019-11-14T10:53:54Z

@cool-RR Thanks for your review. Could you tell me where I can find the necessary value for length args (or where can I set variable for maximum length args)?

cool-RR

Good work, I left a bunch of comments.

cool-RR

Looks good to me!

dorosch · 2019-11-15T07:56:01Z

Looks good to me!

Thanks for your code review)

taleinat

Thanks for the PR! It is certainly in the right direction :)

bedevere-bot · 2019-11-15T08:14:42Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

dorosch · 2019-11-15T09:26:42Z

I have made the requested changes; please review again

bedevere-bot · 2019-11-15T09:26:45Z

Thanks for making the requested changes!

@taleinat: please review the changes made to this pull request.

taleinat

Thanks for making the changes so quickly!

This is in the right direction but I have a few more requests.

taleinat · 2019-11-15T19:12:53Z

+
+        return (
+            f"<{self.__class__.__name__}: "
+            f"returncode:'{self.returncode}' args:'{args}'>"


I think the return code should not be surrounded by quotes: it will be either an integer or None, but never a string.

I'm not sure about args, but if we go with putting quotes around it, that would be interpreted as being a string literal, so we should use its repr, i.e. args:{args!r}.

I agree regarding the return code.

Also regarding the args: I suggested map(shlex.quote, args) but I know there's also subprocess.list2cmdline which has different results. I picked the former arbitrarily, if you have reason to prefer the latter, go for it.

In any case, @dorosch should definitely remove the quotes he included and use !r as you suggested.

Note that list2cmdline() is only for use on Windows!!

It may be wise to use it if _mswindows is true, falling back to ' '.join(map(shlex.quote, args)) otherwise.

Or just print the args as a list and avoid the whole issue:

f'<Popen: ... args: {list(self.args)!r}>

Note that list2cmdline() is only for use on Windows!!

That was worth knowing! Thanks.

In that case, my recommendation is to stay with the shlex code that's in the PR. I'd avoid printing the args as a list, it's always annoying to see these lists when debugging. (Of course, they should always be used when calling subprocess functions.)

Yes, command lines are easier to read when not written as lists. On the other hand, a list matches the interface of the Popen() constructor and the internal representation, is not platform-dependent, and is unambiguous.

It's a good sign that everything is settling down and we're left just with "bike-shedding" ;) The proper place for this discussion would be the bug tracker. Care to bring this up there, @cool-RR?

Nah, it's indeed bikeshedding and we can let it go. After removing the shlex imports, this PR is good to go.

bedevere-bot · 2019-11-16T09:12:21Z

Thanks for making the requested changes!

@taleinat: please review the changes made to this pull request.

taleinat

Both of the shlex imports are no longer necessary.

Other than that, is LGTM!

bedevere-bot · 2019-11-16T20:07:28Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

dorosch · 2019-11-17T06:22:45Z

Both of the shlex imports are no longer necessary.

Other than that, is LGTM!

Thank you for your comment

dorosch · 2019-11-17T06:23:00Z

I have made the requested changes; please review again

bedevere-bot · 2019-11-17T06:23:04Z

Thanks for making the requested changes!

@taleinat: please review the changes made to this pull request.

taleinat

LGTM

taleinat · 2019-11-17T09:45:29Z

@gpshead, @giampaolo, want to take a look at this before it goes in?

giampaolo

LGTM. Good change.

taleinat · 2019-11-17T14:08:18Z

Thanks for the extra review @giampaolo!

taleinat · 2019-11-17T14:11:06Z

Also, thanks @cool-RR for the original suggestion and for the code reviews here!

Implement subprocess.Popen.__repr__

0d3df1c

dorosch requested a review from gpshead as a code owner November 14, 2019 10:30

the-knights-who-say-ni added the CLA not signed label Nov 14, 2019

bedevere-bot added the awaiting review label Nov 14, 2019

cool-RR reviewed Nov 14, 2019

View reviewed changes

Update after review

081c1c3

cool-RR reviewed Nov 14, 2019

View reviewed changes

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Nov 14, 2019

Fix tests and change on format string

bd0bcca

cool-RR approved these changes Nov 15, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Nov 15, 2019

taleinat requested changes Nov 15, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Nov 15, 2019

Update after review

25d7956

bedevere-bot added awaiting change review and removed awaiting changes labels Nov 15, 2019

bedevere-bot requested a review from taleinat November 15, 2019 09:26

brandtbucher added the type-feature A feature request or enhancement label Nov 15, 2019

taleinat requested changes Nov 15, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Nov 15, 2019

bedevere-bot requested a review from taleinat November 16, 2019 09:12

taleinat requested changes Nov 16, 2019

View reviewed changes

bedevere-bot removed the awaiting change review label Nov 16, 2019

bedevere-bot added the awaiting changes label Nov 16, 2019

dorosch added 2 commits November 17, 2019 09:18

Merge branch 'master' into implement-subprocess.Popen.__repr__

bd3e7ef

Remove unused import

519b8d0

bedevere-bot added awaiting change review and removed awaiting changes labels Nov 17, 2019

bedevere-bot requested a review from taleinat November 17, 2019 06:23

taleinat added 2 commits November 17, 2019 09:13

cleanup test a bit

0464fa8

fix failing test

efc0c55

taleinat approved these changes Nov 17, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Nov 17, 2019

taleinat requested a review from giampaolo November 17, 2019 09:44

giampaolo approved these changes Nov 17, 2019

View reviewed changes

taleinat merged commit 645005e into python:master Nov 17, 2019

bedevere-bot removed the awaiting merge label Nov 17, 2019

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019

bpo-38724: Implement subprocess.Popen.__repr__ (pythonGH-17151)

193e828

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020

bpo-38724: Implement subprocess.Popen.__repr__ (pythonGH-17151)

dc5ffc1

Uh oh!

Uh oh!

Conversation

dorosch commented Nov 14, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Nov 14, 2019

Recognized GitHub username

Uh oh!

cool-RR left a comment

Choose a reason for hiding this comment

Uh oh!

dorosch commented Nov 14, 2019

Uh oh!

cool-RR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cool-RR left a comment

Choose a reason for hiding this comment

Uh oh!

dorosch commented Nov 15, 2019

Uh oh!

taleinat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Nov 15, 2019

Uh oh!

dorosch commented Nov 15, 2019

Uh oh!

bedevere-bot commented Nov 15, 2019

Uh oh!

taleinat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

taleinat Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

cool-RR Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

taleinat Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

cool-RR Nov 16, 2019

Choose a reason for hiding this comment

Uh oh!

taleinat Nov 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cool-RR Nov 17, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Nov 16, 2019

Uh oh!

taleinat left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Nov 16, 2019

Uh oh!

dorosch commented Nov 17, 2019

Uh oh!

dorosch commented Nov 17, 2019

Uh oh!

dorosch commented Nov 14, 2019 •

edited by bedevere-bot

Loading

taleinat Nov 16, 2019 •

edited

Loading